Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use SchemaReflection and BoundSchemaReflection instead of SchemaCache #384

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jwoodrow
Copy link

@jwoodrow jwoodrow commented Nov 12, 2023

Rails 7.1 warns that ActiveRecord::ConnectionAdapters::SchemaCache will be removed in Rails 7.2.

I tried to look around at what was being returned by ActiveRecord::ConnectionAdapters::SchemaCache and try to rebuild the exact same class and this seems to work out.

Ran the CI on our end and it seems to work if I use the ActiveRecord version as a condition for the switch. There's most-likely a better way of doing this if you have suggestions.


Solves #382

@jwoodrow jwoodrow changed the title User SchemaReflection and BoundSchemaReflection instead of SchemaCache Use SchemaReflection and BoundSchemaReflection instead of SchemaCache Nov 12, 2023
@@ -212,7 +212,12 @@ def initialize(proxy)
@proxy = proxy
@owner = nil
@pool = nil
@schema_cache = ActiveRecord::ConnectionAdapters::SchemaCache.new @proxy
if ActiveRecord.version >= Gem::Version.new('7.1.0')
schema_reflection = ActiveRecord::ConnectionAdapters::SchemaReflection.new(nil).load!(@proxy)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still not going to work because you are passing nil to ActiveRecord::ConnectionAdapters::SchemaReflection.new

The right approach would be passing the schema_cache_path to ActiveRecord::ConnectionAdapters::SchemaReflection.new in order to fix it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants